New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix precedence of ui.virtual.whitespace #8750

Merged

archseer merged 3 commits into helix-editor:master from chtenb:whitespace_highlighting_precedence2

Nov 22, 2023

Contributor

chtenb commented Nov 7, 2023

This fixes the first half of #5675, namely the whitespace case.
Based on the suggestions by @pascalkuthe in #8733
Is this along the lines of what you envisioned?

chtenb added 2 commits

November 7, 2023 16:50


          Separate iters for overlay and syntax highlighting.

92e6932

This allows for more control over the order (precedence) in which styles are patched in the rendering phase.


          Fix clippy warning

d925c0c

kirawi added the A-helix-term label

pascalkuthe reviewed

View reviewed changes

Member

pascalkuthe left a comment

this looks pretty good, this will also help with some other stuff in the future. I may be able to use this to finally fix a bunch of other highlight issues in the future

helix-term/src/ui/document.rs Outdated Show resolved Hide resolved

helix-term/src/ui/document.rs Outdated Show resolved Hide resolved

helix-term/src/ui/editor.rs Show resolved Hide resolved

pascalkuthe added S-waiting-on-author E-easy labels


          Address feedback

c44513d

Contributor Author

chtenb commented Nov 21, 2023

I pushed a commit addressing the points you made.

pascalkuthe approved these changes

View reviewed changes

Contributor Author

chtenb commented Nov 21, 2023

I think the label "waiting-on-author" can be removed.

gabydd added S-waiting-on-review and removed S-waiting-on-author labels

archseer merged commit 41b307b into helix-editor:master

6 checks passed

tanj commented Nov 22, 2023 •

edited

Loading

not sure if there is anything special about this, but when I built with this commit merged to master I lose all syntax highlighting. If I go back one commit to 47b6c4b and build syntax highlighting is back.

This is built on Windows 11 and running in Windows Terminal

gabydd reviewed

View reviewed changes

helix-term/src/ui/document.rs

+                      theme,
+                  };
+                  let mut overlay_styles = StyleIter {
+                      text_style: renderer.text_style,

Member

gabydd Nov 22, 2023

using render.text_style is wrong here it will set the whole document to the text colour as the overlay is patched last, this should just be Style::default so if there is no style here we don't override the past text highlighting

Member

gabydd Nov 22, 2023

see StyleIter for why it will highlight all the text the default text color:

helix/helix-term/src/ui/document.rs

Lines 64 to 66 in 41b307b

    
           .fold(self.text_style, |acc, span| { 
        
               acc.patch(self.theme.highlight(span.0)) 
        
           });

as overlay_highlight_iter covers the whole viewport

Contributor Author

chtenb Nov 22, 2023

Looking into this more deeply, this text_style is set based on the given ui.text scope. The documentation about this says "Command prompts, popup text, etc.", but as this is really the default text style that is used, we should probably change that description.

Member

archseer commented Nov 22, 2023

Reverting. I thought this PR was properly tested.

archseer added a commit that referenced this pull request


          Revert "Fix precedence of ui.virtual.whitespace (#8750)"

811d62d

This reverts commit 41b307b.

Contributor Author

chtenb commented Nov 22, 2023 •

edited

Loading

My apologies for missing this. I had been using this patch for weeks without problems. It seems to be that the themes I was using did not expose this issue. But other themes, like the default theme, do expose this issue. I will fix the problem and make sure to test with a wider variety of themes.

EDIT: this is probably related to whether the theme sets the ui.text scope or not.

chtenb added a commit to chtenb/helix that referenced this pull request


          Revert "Revert "Fix precedence of ui.virtual.whitespace (helix-editor…

6f59184

…#8750)""

This reverts commit 811d62d.

chtenb mentioned this pull request

Fix precedence of ui.virtual.whitespace #8879

Merged

pascalkuthe pushed a commit that referenced this pull request


          Fix precedence of ui.virtual.whitespace (#8879)

8c68074

* Revert "Revert "Fix precedence of ui.virtual.whitespace (#8750)""

This reverts commit 811d62d.

* Fix ui.text overwriting the syntax highlighting

Adjust ui.text description

dgkf pushed a commit to dgkf/helix that referenced this pull request


          Fix precedence of ui.virtual.whitespace (helix-editor#8750)

2238ce6

dgkf pushed a commit to dgkf/helix that referenced this pull request


          Revert "Fix precedence of ui.virtual.whitespace (helix-editor#8750)"

de0bd39

This reverts commit 41b307b.

dgkf pushed a commit to dgkf/helix that referenced this pull request


          Fix precedence of ui.virtual.whitespace (helix-editor#8879)

c1cdd1a

* Revert "Revert "Fix precedence of ui.virtual.whitespace (helix-editor#8750)""

This reverts commit 811d62d.

* Fix ui.text overwriting the syntax highlighting

Adjust ui.text description

mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request


          Fix precedence of ui.virtual.whitespace (helix-editor#8750)

3eb6c5a

mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request


          Revert "Fix precedence of ui.virtual.whitespace (helix-editor#8750)"

97e2110

This reverts commit 41b307b.

mtoohey31 pushed a commit to mtoohey31/helix that referenced this pull request


          Fix precedence of ui.virtual.whitespace (helix-editor#8879)

2be817c

* Revert "Revert "Fix precedence of ui.virtual.whitespace (helix-editor#8750)""

This reverts commit 811d62d.

* Fix ui.text overwriting the syntax highlighting

Adjust ui.text description

Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request


          Fix precedence of ui.virtual.whitespace (helix-editor#8750)

44fb2be

Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request


          Revert "Fix precedence of ui.virtual.whitespace (helix-editor#8750)"

25b3e1a

This reverts commit 41b307b.

Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request


          Fix precedence of ui.virtual.whitespace (helix-editor#8879)

* Revert "Revert "Fix precedence of ui.virtual.whitespace (helix-editor#8750)""

This reverts commit 811d62d.

* Fix ui.text overwriting the syntax highlighting

Adjust ui.text description

smortime pushed a commit to smortime/helix that referenced this pull request


          Fix precedence of ui.virtual.whitespace (helix-editor#8750)

24af082

smortime pushed a commit to smortime/helix that referenced this pull request


          Revert "Fix precedence of ui.virtual.whitespace (helix-editor#8750)"

51e16d5

This reverts commit 41b307b.

smortime pushed a commit to smortime/helix that referenced this pull request


          Fix precedence of ui.virtual.whitespace (helix-editor#8879)

46ca8d1

* Revert "Revert "Fix precedence of ui.virtual.whitespace (helix-editor#8750)""

This reverts commit 811d62d.

* Fix ui.text overwriting the syntax highlighting

Adjust ui.text description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-helix-term E-easy S-waiting-on-review